Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better settings parsing and pip installation handling #106

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Conversation

b-rowan
Copy link
Contributor

@b-rowan b-rowan commented Aug 29, 2024

Hopefully a better solution to settings parsing than #105, uses platformdirs to find device specific directories where possible. Will also log a warning about missing settings file, but will still check in project root for development.

Also moved main.py to __main__.py for better pip installation support, so it can be run with python -m goosebit.

Fixes: #103

goosebit/settings/const.py Outdated Show resolved Hide resolved
goosebit/settings/const.py Outdated Show resolved Hide resolved
goosebit/settings/const.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@easybe easybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And of course some nagging about the commit messages. I appreciate that they now contain more information. But, reading them in the console is not that great:

Screenshot from 2024-08-31 23-02-42

That is why one should break the lines, as explained here. A common guide line is 50 characters for the subject and 72 for the body. Pro tip: Editors can help you with that.

So, your commit message I would change to something like this:

Check multiple places for settings file

Also, rename `settings.yaml` to `goosebit.yaml` to make it
distinguishable from other files.

The user can now pass `GOOSEBIT_SETTINGS` to customize settings
location, as well as gooseBit auto checking `/etc/goosebit.yaml`, the
root of the project directory, and the current working directory when it
is run. This should allow for more optionality when loading settings
files.

Fixes: #103

goosebit/settings/const.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
@b-rowan
Copy link
Contributor Author

b-rowan commented Aug 31, 2024

And of course some nagging about the commit messages. I appreciate that they now contain more information. But, reading them in the console is not that great:

Screenshot from 2024-08-31 23-02-42

That is why one should break the lines, as explained here. A common guide line is 50 characters for the subject and 72 for the body. Pro tip: Editors can help you with that.

So, your commit message I would change to something like this:


Check multiple places for settings file



Also, rename settings.yaml to `goosebit.yaml` to make it distinguishable

from other files.



Use can now pass `GOOSEBIT_SETTINGS` to customize settings location, as

well as gooseBit auto checking `/etc/goosebit.yaml`, the root of the

project directory, and the current working directory when it is run.

This should allow for more optionality when loading settings files.

    

Fixes: #103

I wonder if this is a good case for pre-commit?

@easybe
Copy link
Collaborator

easybe commented Aug 31, 2024

And of course some nagging about the commit messages. I appreciate that they now contain more information. But, reading them in the console is not that great:
Screenshot from 2024-08-31 23-02-42
That is why one should break the lines, as explained here. A common guide line is 50 characters for the subject and 72 for the body. Pro tip: Editors can help you with that.
So, your commit message I would change to something like this:


Check multiple places for settings file



Also, rename settings.yaml to `goosebit.yaml` to make it distinguishable

from other files.



Use can now pass `GOOSEBIT_SETTINGS` to customize settings location, as

well as gooseBit auto checking `/etc/goosebit.yaml`, the root of the

project directory, and the current working directory when it is run.

This should allow for more optionality when loading settings files.

    

Fixes: #103

I wonder if this is a good case for pre-commit?

There is https://github.com/jorisroovers/gitlint

@b-rowan b-rowan force-pushed the dev_settings branch 2 times, most recently from 93c2429 to a1f4cc2 Compare August 31, 2024 22:40
@b-rowan
Copy link
Contributor Author

b-rowan commented Aug 31, 2024

And of course some nagging about the commit messages. I appreciate that they now contain more information. But, reading them in the console is not that great:
Screenshot from 2024-08-31 23-02-42
That is why one should break the lines, as explained here. A common guide line is 50 characters for the subject and 72 for the body. Pro tip: Editors can help you with that.
So, your commit message I would change to something like this:


Check multiple places for settings file



Also, rename settings.yaml to `goosebit.yaml` to make it distinguishable

from other files.



Use can now pass `GOOSEBIT_SETTINGS` to customize settings location, as

well as gooseBit auto checking `/etc/goosebit.yaml`, the root of the

project directory, and the current working directory when it is run.

This should allow for more optionality when loading settings files.

    

Fixes: #103

I wonder if this is a good case for pre-commit?

There is https://github.com/jorisroovers/gitlint

#110

@easybe
Copy link
Collaborator

easybe commented Aug 31, 2024

I'll want to test this...

@easybe easybe self-requested a review August 31, 2024 23:00
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Outdated Show resolved Hide resolved
@b-rowan b-rowan requested a review from easybe September 4, 2024 14:36
Copy link
Collaborator

@easybe easybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I would squash the last commit. No need for intermediate (broken) versions.

goosebit/logging/__init__.py Outdated Show resolved Hide resolved
goosebit/settings/schema.py Show resolved Hide resolved
@b-rowan
Copy link
Contributor Author

b-rowan commented Sep 4, 2024

And I would squash the last commit. No need for intermediate (broken) versions.

Will do, just give me a bit.

goosebit/logging/__init__.py Outdated Show resolved Hide resolved
@UpstreamData UpstreamData force-pushed the dev_settings branch 2 times, most recently from 8639a81 to 7064c53 Compare September 4, 2024 15:42
…settings.

Use can now pass `GOOSEBIT_SETTINGS` to customize settings location, as well as goosebit auto checking `/etc/goosebit` if it exists, the root of the project directory, and the current working directory when it is run.
This should allow for more optionality when loading settings files.

Renaming settings to `goosebit.yaml` is mainly to specify what the file is when running from CWD, it should be distinct from other files.

Fixes: #103
Copy link
Collaborator

@easybe easybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement, thanks!

@b-rowan b-rowan merged commit e24017e into master Sep 4, 2024
5 checks passed
@b-rowan b-rowan deleted the dev_settings branch September 4, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix storage location of settings.yaml
3 participants